Feat/llm retry report:SDK retry-attempt observability for review (#785) - #790

Thank you, but this is big and hard to review.

✅ OpenCodeReview: Review partially complete: 0 finding(s); 2 of 16 selected item(s) failed.

Thank you, but this is big and hard to review.

Hi @wu21-web, Thanks for the feedback! To help with the review, here's a breakdown — about 4,670 of the 6,197 added lines (~75%) are tests. The core implementation is only ~1,500 lines:

New core files (suggested reading order):

1. internal/llm/retry_meta.go (+154) — retry metadata carried per request
2. internal/llm/retry_boundary.go (+141) — retry boundary decisions
3. internal/llm/retry_observer.go (+132) — observes retry attempts from the SDK
4. internal/llm/retry_report.go (+658) — aggregates observations into the report

Integration points:

• internal/llm/client.go (+97) and internal/llm/responses_client.go (+27) — hook observers into the SDK clients
• internal/llmloop/loop.go (+70) — thread retry metadata through the loop
• internal/agent/agent.go (+40) — propagate to agents
• cmd/opencodereview/output.go (+79) and cmd/opencodereview/review_cmd.go (+32) — render the report

Really solid piece of work — thanks for the layered commits and for writing down the invariants instead of just asserting them in the PR description. A few notes from going through it carefully.

Things I verified rather than assumed, since they're load-bearing for the whole design:

• The middleware genuinely sits inside the SDK retry loop. requestconfig.go:419-423 builds handler from the middleware chain, and res, err = handler(req) is called at line 449 inside for retryCount := 0; retryCount <= cfg.MaxRetries. Same shape in openai-go v3. So one observer invocation per real HTTP attempt holds.
• shouldRetry (requestconfig.go:248) does consult x-should-retry ahead of the status code and does not exclude 2xx — your OutcomeSucceeded comment about "nothing was recovered from" is describing a real case, not a hypothetical.
• The per-attempt timeout path returns the derived ctx's DeadlineExceeded, while finalizeRequest reads the parent ctx for parentCancelled. So an attempt-level timeout correctly lands on failed rather than cancelled, exactly as your design note claims.
• go build, go vet, and go test -race -count=1 across internal/{llm,llmloop,diff,agent,scan,session} + cmd/opencodereview all pass on my machine.

The one thing I'd like to see changed: cancelled is counted in failed_requests

retry_report.go:528 folds OutcomeCancelled into FailedRequests, and RetryReport has no cancelled_requests field. That matters more than it looks, because cancelling an in-flight background compression isn't an edge case — it's a designed-for, routine event. Beyond the deferred cancelPendingCompression at loop.go:237, there are two in-loop call sites (loop.go:569 and loop.go:590): whenever the async job hasn't landed by the time the conversation crosses the warning threshold, you cancel it and fall back to synchronous compression.

The full chain: asyncCtx = WithTimeout(WithoutCancel(ctx), 5min) → cancel() → the in-flight request aborts, http.Client.Do returns (nil, *url.Error{Err: context.Canceled}) → the observer records an attempt with StatusCode == 0 classified cancelled/context → finalizeRequest sees ctx.Err() == context.Canceled → OutcomeCancelled → FailedRequests++, and it gets listed because outcome != succeeded.

Net effect: a run with zero retries and zero genuine failures can print

LLM retry report: 0/12 requests retried, 0 retries, 0 recovered, 1 failed
- internal/foo.go / memory_compression #1: cancelled -> cancelled

The classification itself is right — it really was cancelled. It's the aggregate label that's wrong, and cancelled -> cancelled reads oddly too (the attempt has no status code, so retryAttemptChain emits the bare class and then appends the request outcome). I'd add cancelled_requests and keep failed_requests for OutcomeFailed only. A user-initiated Ctrl-C on main_task is still worth surfacing, so this is about splitting the counter, not hiding anything.

Two latent things — both unreachable today, so entirely your call

I'm flagging these as code-level inconsistencies, not as bugs. I checked reachability on both and couldn't get either to fire in production.

freezeErr reaches the process exit code. review_cmd.go:284 returns errors.Join(emitErr, freezeErr), which lands on main.go:27's os.Exit(1). On a fully successful run, stdout would carry a complete "status": "success" document while the process exits non-zero — awkward for CI. That said, I walked every Freeze error exit and none of them is reachable: runID is always non-empty (session.New unconditionally does sessionID := generateUUID(), history.go:151); the violation strings need either duplicate RequestMeta identity (impossible — RequestNo is allocated under fs.mu, scoped per (FilePath, TaskType)) or an attempt after the boundary defer (impossible — both the EOF re-call and the streaming path complete before it); non-2xx without a classification can't happen because observeAttempt is the only production caller and always classifies; and not finalized is closed off by WaitBackground. Your own var newRetryCollector hook exists for exactly this reason, so we agree on the facts. My only suggestion is that a pure-observability invariant breach probably shouldn't be able to fail an otherwise-successful review — a stderr warning would be a softer landing. Not a blocker either way.

isErrorStatus vs the SDKs' >= 400. retry_report.go:172 treats anything outside 200–299 as proof of failure, while both SDKs use res.StatusCode >= 400. I initially thought a 3xx would show up as a false error attempt and flip a successful request to recovered, but that doesn't hold: both SDKs populate GetBody (requestconfig.go:401 / :434), so http.Client follows 301/302/303/307/308 and the middleware only ever sees the post-redirect response. The statuses Go won't follow (300/304/305/306) don't occur against an LLM API — no conditional requests are sent, and Go rejects 305 outright. And even if a 300 did arrive, the SDK would try to JSON-decode it, fail, and classifyBoundaryError would land the request on failed, not a bogus recovered. So: real divergence, no reachable consequence. Might still be worth pinning to >= 400, or adding a line explaining why you're deliberately stricter than the SDK, since the "a non-2xx status is authoritative" framing in the PR body reads as if it matches the SDK boundary when it doesn't.

Small stuff

• WaitBackground fixes more than the PR claims credit for. Because it runs ahead of finalizeManifest() and session.Finalize(), a background compression goroutine can no longer write llm_request/llm_response lines after the session file has been finalized. That was a pre-existing race on the review path and you've closed it — worth a sentence in the PR body, since a reviewer skimming for "why is this needed" will only see the retry-report justification.
• internal/scan still has that race. scan/agent.go:335,371 call a.session.Finalize() but nothing calls WaitBackground. Out of scope here, and the nil NewRequestMeta comment is clear about why scan opts out of the report — but a line in WaitBackground's doc saying "scan deliberately doesn't join, since it freezes no report" would stop the next reader from thinking it's an oversight.
• Comment parity on re-location. compression.go:230 explains the record-before-request reordering and its resume implication in detail. loop.go:470 makes the same reordering but only explains the cm.Path choice. Behaviour is equivalent (I diffed the old and new msgs semantics — BuildReLocationMessages returns nil for exactly the cases the old early return covered), so this is purely a cross-reference. Also worth noting that rlStart now includes prompt construction and AppendTaskRecord in TaskRecord.Duration — negligible, but the comment says moving it would change what Duration measures, when leaving it in place changed it slightly too.
• Docs. The top-level field table in pages/src/content/docs/{en,ja,ru,zh}/cli-reference.md doesn't list retry_report. It's already missing manifest from Emit a complete immutable review manifest and partial-coverage contract #367, so this isn't new debt you're creating, and omitempty keeps the JSON contract additive — but the terminal block is unconditionally user-visible. Deferring until Add trusted resume with explicit provider and model transition lineage #786/Add provider-specific retry budgets and a shared retry coordinator #787 land is a reasonable call; I'd just open a tracking issue so the backlog doesn't grow another entry.

One retraction on my side: I first questioned outputRetryReportText(os.Stdout, ...) versus the stdout.Writer() used elsewhere. That was wrong — newQuietHandle silences the stdout package's progress writer, not os.Stdout, and result output should bypass it exactly the way outputTextWithWarnings and the project summary already do. Your version is correct.

Overall: the only change I'd ask for before merge is splitting cancelled_requests out of failed_requests. Everything else is comments, docs, or optional tightening.

Thanks @lizhengfeng101 for the detailed review. I addressed the merge-blocking point in 5f99e5f: cancelled_requests is now separate from failed_requests, cancelled requests remain visible, and the duplicated cancelled -> cancelled rendering is removed. I also updated the related validation, tests, listing-rule comment, and the PR body’s WaitBackground race note. The two unreachable latent issues and the other optional items remain unchanged to keep this follow-up minimal. make check and the full race-enabled make test pass locally.